Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor NonZero, Shared, and Unique APIs #41064

Merged
merged 4 commits into from
May 6, 2017
Merged

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Apr 4, 2017

Major difference is that I removed Deref impls, as apparently LLVM has
trouble maintaining metadata with a &ptr -> &ptr API. This was cited
as a blocker for ever stabilizing this API. It wasn't that ergonomic
anyway.

  • Added get to NonZero to replace Deref impl
  • Added ptr getter to Shared/Unique to replace Deref impl
  • Added Unique's get and get_mut conveniences to Shared
  • Deprecated as_mut_ptr on Shared in favour of ptr

Note that Shared used to primarily expose only *const but there isn't
a good justification for that, so I made it *mut.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Gankra
Copy link
Contributor Author

Gankra commented Apr 4, 2017

CC @eddyb who brought up the Deref issue.

// the contract anyway.
// This allows the null check to be elided in the destructor if we
// manipulated the reference count in the same function.
assume(!(*(&self.ptr as *const _ as *const *const ())).is_null());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I removed these assumes because they should be implied from NonZero, and the point of these changes is to make that metadata propagate more readily. Should probably try to validate that somehow? Not sure the best way forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can test this out by taking a look at the IR for:

fn foo(r: &Rc<i32>) {
    drop(r.clone());
}

Ideally that's a noop function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may need to copy it to NonZero::get (if you can), at least until we have it in the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned that copying an assume to NonZero::get will be disastrous to compile times (as this will show up in all accesses to every collection). Have those bloat problems with assume been resolved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's a good question (sorry I missed this). cc @dotdash @nagisa @arielb1

@Gankra
Copy link
Contributor Author

Gankra commented Apr 4, 2017

  • TODO: update the nomicon to reflect these changes. Waiting for feedback first.

@Gankra
Copy link
Contributor Author

Gankra commented Apr 4, 2017

Oh I also made Unique Copy/Clone, because it's unclear what value move semantics has here.

@SimonSapin
Copy link
Contributor

Oh I also made Unique Copy/Clone, because it's unclear what value move semantics has here.

This is kind of a weak argument, but copying or cloning seems contrary to the name "unique" or the docs "indicates that the possessor of this wrapper owns the referent".

And is this useful? Something that contains Unique<_> owns the allocation, so it’s responsible for deallocating, so it needs to implements Drop, so it can’t implement Copy. As to cloning such a thing, that is done by allocating new memory, cloning the contents, and using Unique::new with the new pointer.

@Gankra
Copy link
Contributor Author

Gankra commented Apr 4, 2017

Primary motivation for Copyable Unique was to have a by-value getter and maybe tax the optimizer a tiny bit less. Also to make it more of a drop-in-replacement for *mut; possibly if you're just trying to use it as an optimizer hint?

I'm not particularly passionate about it either way. Maybe @nikomatsakis can speak to aliasing model implications?

@Ericson2314
Copy link
Contributor

I appreciate not stressing the optimizer, but doing that does make for an annoying footgun.

@Gankra
Copy link
Contributor Author

Gankra commented Apr 4, 2017

Every Unique lives in a struct that owns it, so I don't know what the footgun could possibly be; especially since using the Unique requires you to immediately turn it into a primitive pointer that can be copied.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Apr 4, 2017

It's been a while, but I vaguely recall using "raw" Uniques not inside some other struct as a last-ditch attempt to prevent unwanted aliasing.

Even if the wrapped struct case, there is still code working with the wrapper.

pub fn ptr(self) -> *mut T {
self.pointer.get() as *mut _
}

/// Dereferences the content.
pub unsafe fn get(&self) -> &T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may call this get_ref nowadays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about ref and ref_mut?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref is a keyword

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 4, 2017
@alexcrichton
Copy link
Member

These are so low-level I'm ok with adding Clone and Copy. It's not safe to use these types with or without those trait bounds, and with them it's just a little easier to work with Unique and Shared

@bors
Copy link
Contributor

bors commented Apr 6, 2017

☔ The latest upstream changes (presumably #41098) made this pull request unmergeable. Please resolve the merge conflicts.

&*self.ptr()
}

/// Mutably dereferences the content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be on Shared at all? I'd expect interior mutability to be required - or I suppose we could just have something along the lines "you shouldn't ever use this unless there is no other reference around" in the documentation (e.g. Rc with a refcount of 1).

@eddyb
Copy link
Member

eddyb commented Apr 6, 2017

Note that Shared used to primarily expose only *const but there isn't a good justification for that, so I made it *mut.

That's a breaking change AFAIK, I think it's for variance.

@Gankra
Copy link
Contributor Author

Gankra commented Apr 7, 2017

@eddyb it still contains a *const; it's just that there's no API that goes -> *const anymore, because that's just making developers jump through hoops.

Shared has, basically, no semantics other than "it's non-null" and "it doesn't have *mut's useless variance". In other words, it's the unsafe pointer that everyone wishes existed. I don't know where you'd expect to put any kind of interior mutability annotation; certainly Arc and Rc haven't bothered to do any such thing.

It's possible there's interesting aliasing rules to document here, but until someone figures out what Rust's aliasing rules are, that's simply impossible.

@eddyb
Copy link
Member

eddyb commented Apr 7, 2017

@gankro Ahh, okay, keeping *const T in the struct and returning *mut T is fine.

The refcounts are Cells in Rc and AtomicUsizes in Arc, that's what I was referring to, and that all user data has to be wrapped in interior mutability to get a &mut, with the notable exception of make_mut, which is presumably the only user of Shared::get_mut in Rc/Arc.
If it's for symmetry, I'm fine with it, although the usecase is so rare I'd almost have it be done with .ptr().

@nikomatsakis
Copy link
Contributor

Maybe @nikomatsakis can speak to aliasing model implications?

I can't speak to it with certainty, except to say that I think there is pretty broad consensus that errors ought to be mostly about which pointers get dereferenced and when, not which get copied from place to place.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@carols10cents
Copy link
Member

Ping to keep this on your radar @gankro!

@Gankra
Copy link
Contributor Author

Gankra commented Apr 17, 2017

I'm not sure this is really blocked on me. The only real requested change is from @alexcrichton, but really the blocker is on libs/lang team people agreeing on the design.

@Gankra
Copy link
Contributor Author

Gankra commented Apr 17, 2017

Another API question that I kinda spaced out on: the current design does (&self) -> &T so the lifetime is bounded. This works well when the ptr is in self, but can behave poorly otherwise (see LinkedList's code). The alternative is to do (self) -> &T and return an unbounded lifetime. Maybe this is a case where we want both? ref() and ref_unbound()?

@carols10cents
Copy link
Member

@gankro Oh I thought it should be in your court because there are merge conflicts and test failures. Are you waiting on approval from libs/lang on the design before fixing those?

@Gankra
Copy link
Contributor Author

Gankra commented Apr 17, 2017

Yeah

@carols10cents carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 17, 2017
@alexcrichton
Copy link
Member

I'll cc @rust-lang/libs to see if anyone else would like to leave a comment, but typically if we're not stabilizing something we don't discuss PRs during triage (as it's just too much to process).

I would be comfortable r+'ing w/ my minor comment and a rebase.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 5, 2017
refactor NonZero, Shared, and Unique APIs

Major difference is that I removed Deref impls, as apparently LLVM has
trouble maintaining metadata with a `&ptr -> &ptr` API. This was cited
as a blocker for ever stabilizing this API. It wasn't that ergonomic
anyway.

* Added `get` to NonZero to replace Deref impl
* Added `ptr` getter to Shared/Unique to replace Deref impl
* Added Unique's `get` and `get_mut` conveniences to Shared
* Deprecated `as_mut_ptr` on Shared in favour of `ptr`

Note that Shared used to primarily expose only `*const` but there isn't
a good justification for that, so I made it `*mut`.
bors added a commit that referenced this pull request May 5, 2017
Rollup of 9 pull requests

- Successful merges: #41064, #41307, #41512, #41582, #41678, #41722, #41734, #41761, #41763
- Failed merges:
@bors
Copy link
Contributor

bors commented May 6, 2017

⌛ Testing commit e8234e0 with merge 42a4f37...

@twmb
Copy link

twmb commented May 12, 2017

I believe the documentation needs updated to no longer refer to ptr and ptr_mut, and instead to refer to as_ptr and as_mut_ptr.

hawkw added a commit to sos-os/kernel that referenced this pull request May 25, 2017
kevincox added a commit to kevincox/rust-gc that referenced this pull request Jul 22, 2017
kevincox added a commit to kevincox/rust-gc that referenced this pull request Jul 22, 2017
kevincox added a commit to kevincox/rust-gc that referenced this pull request Jul 22, 2017
ids1024 added a commit to ids1024/ralloc that referenced this pull request Nov 8, 2017
- New allocator API
- Remove the "allocator" feature, which should be unnecessary due to how
the new allocator API works
- NonZero no longer implements Deref (rust-lang/rust#41064)
- NonZero::new() returns an Option; use NonZero::new_unchecked()
- Thread locals are no longer 'static (rust-lang/rust#43746)
- Changes to feature flags
- Use unsafe to access extern static (rust-lang/rust#36247)
SimonSapin added a commit to SimonSapin/rust that referenced this pull request Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.